Repository navigation
fix(heading): keep table_info in set_primary_key so a later rename quotes for the backend - #1643
Conversation
…otes for the backend Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Thanks, @mayuriphad. This is the fix for #1638, and the root cause matches what I found when filing it: Fork PRs don't trigger CI in this repository at the moment, so I ran it locally on current
66 passed in all. A maintainer will follow up on the review. |
dimitri-yatsenko
added
the
bug
MilagrosMarin
left a comment
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks @mayuriphad, clean and well-scoped fix.
✅ Passing table_info matches what Heading.select already does when it builds its heading
✅ Nice test: it reproduces the issue without needing a database
One possible follow-up (not blocking this PR): Heading.join and Heading.make_subquery_heading also build a new Heading without table_info. Reading the code, a rename after a join, e.g. (A * B).proj(c="b") or a projection of an .aggr result, looks like it would fall back to backticks on PostgreSQL in the same way. I haven't run it to confirm. Worth a quick check, and a separate issue if it reproduces?
LGTM otherwise.
dimitri-yatsenko
left a comment
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
- This matches
Heading.select(), which already carriestable_infoforward, and the test reproduces #1638 without a database. - The follow-up already raised holds:
Heading.join()(heading.py:756) andmake_subquery_heading()(:776) also build aHeadingwithouttable_info. Worth a follow-up issue, with the same rename test after a join or subquery. - CI hasn't run on this PR yet (first-time contributor workflow approval). The run should be approved and pass before merging.
|
Thanks again, @mayuriphad. GitHub Actions never ran on this PR, because no workflows ran in this repository between Oct 31 and Nov 9. So I merged it onto current The follow-up both reviews raised is real. |
Fixes #1638
Heading.set_primary_keybuilt the new heading withouttable_info, so it lost the connection. When a projection then renamed an attribute of that heading,Heading.selecthad no adapter and fell back to backtick quoting. That is why(dj.U("b") & T).proj(c="b")compiled toSELECT DISTINCT `b` as "c"and failed on PostgreSQL. MySQL accepts backticks, so it hid the problem there.set_primary_keynow passestable_info=self.table_info, the same wayselectalready hands it on to the heading it builds.The new unit test in
tests/unit/test_pk_rules.pygives a heading a PostgreSQL adapter, callsset_primary_key, then renames an attribute. On master the expression comes out as`b`. With the fix it is"b".tests/unitpasses locally (458 passed). I did not run the integration suite because there is no Docker on this machine for the testcontainers.